AsyncRpcContinuation should be disposed #1739
Conversation
|
I have some more thoughts about the code but need to wrap up things for today and they can be handled seperately. |
|
Build fails due to the vulnerability problems mentioned in #1735 and and old version of json. |
lukebakken
force-pushed
the
dispose-continuation
branch
from
5569b40 to
a48b8d1
Compare
… to create timer based cancellation token sources and linked tokens
lukebakken
force-pushed
the
dispose-continuation
branch
from
a48b8d1 to
29e5632
Compare
|
Alright, based on the latest integration test failures, my comment was applicable. We pass the This is why I checked to see if the command was enqueued or not. If enqueued, the handling of the response would (should) take care of disposal, if the RPC command was not enqueued, then dispose the command object. Between checking if the RPC object is enqueued, and this code, instances of Anyways, as is, I think the code works as it should. Let me know what you think. Going to close this PR for now but if there's a way to simplify the code while retaining this behavior that would be great. |
|
Yeah I missed that part. I think if we would put together the creation of the continuation with the acquiring of the semaphore and the enqueuing it would be possible to return a disposable struct that releases the continuation and the RP semaphore or just the RPC semaphore that would reduce nesting and flow logic. I can make a PR to demonstrate it and then we'll see whether it's worth it or not |
|
@lukebakken see #1743 Not sure if it is worth it. It's a bit of a machinery but would make the scope owning explicit and reduce nesting. It would also centralize the decision making more or less into one place That being said I'm not sure overall that it reduces the cognitive overhead |
Proposed Changes
I spent a few minutes looking through the discussion #1721 that @phatboyg raised and saw that even though the RPC continuations are always creating cancellation token sources and linked tokens in the constructor we are not always disposing them which can lead to leaks. It is safer to always dispose and also simplifies the code a bit. There were some comments / notes about not disposing deliberately but I couldn't figure out why that should be the case, so I assumed it is a mistake and introduced disposing.
Types of Changes
What types of changes does your code introduce to this project?
Put an
xin the boxes that applyChecklist
Put an
xin the boxes that apply. You can also fill these out after creatingthe PR. If you're unsure about any of them, don't hesitate to ask on the
mailing list. We're here to help! This is simply a reminder of what we are
going to look for before merging your code.
CONTRIBUTING.mddocumentFurther Comments
If this is a relatively large or complex change, kick off the discussion by
explaining why you chose the solution you did and what alternatives you
considered, etc.